Error reporting improvements - #82
Conversation
|
| /** | ||
| * The error code of the evaluation result. If the evaluation was successful, this will be EvaluationErrorCode.NONE. | ||
| */ | ||
| public EvaluationErrorCode getErrorCode() { | ||
| return errorCode; | ||
| } | ||
|
|
||
| /** | ||
| * The error exception object related to the error. If the evaluation was successful, this will be null. | ||
| */ |
There was a problem hiding this comment.
I suggest using the already UX-approved texts for documentation .
| /** | |
| * The error code of the evaluation result. If the evaluation was successful, this will be EvaluationErrorCode.NONE. | |
| */ | |
| public EvaluationErrorCode getErrorCode() { | |
| return errorCode; | |
| } | |
| /** | |
| * The error exception object related to the error. If the evaluation was successful, this will be null. | |
| */ | |
| /** | |
| * The code identifying the reason for the error in case the operation failed. (If the evaluation was successful, this will be EvaluationErrorCode.NONE.) | |
| */ | |
| public EvaluationErrorCode getErrorCode() { | |
| return errorCode; | |
| } | |
| /** | |
| * The exception object related to the error in case the operation failed. (If the evaluation was successful, this will be null.) | |
| */ |
|
|
||
| public RefreshErrorCode errorCode() { | ||
| return errorCode; | ||
| } | ||
|
|
||
| public Throwable errorException() { | ||
| return errorException; | ||
| } |
There was a problem hiding this comment.
Would be nice to add documentation here too. E.g.: https://github.com/configcat/.net-sdk/blob/v10.0.0/src/ConfigCatClient/RefreshResult.cs#L60-L73
| NONE(0), | ||
|
|
||
| /** | ||
| * The refresh operation failed because the client is configured to use the `OverrideBehaviour.LocalOnly` |
There was a problem hiding this comment.
| * The refresh operation failed because the client is configured to use the `OverrideBehaviour.LocalOnly` | |
| * The refresh operation failed because the client is configured to use the `OverrideBehaviour.LOCAL_ONLY` |
| static <T> Result<T, RefreshErrorCode> success(T value) { | ||
| return new Result<>(value, null, RefreshErrorCode.NONE, null); | ||
| } | ||
|
|
||
| static <T> Result<T> success(T value) { | ||
| return new Result<>(value, null); | ||
| static <T, E extends ErrorCode> Result<T, E> success(T value, E errorCode) { | ||
| return new Result<>(value, null, errorCode, null); | ||
| } |
There was a problem hiding this comment.
It doesn't seem to be a good API design to introduce a factory method only for a specific case in a general class like this. It's also error-prone because naming doesn't make this clear to maintainers. They won't know unless reading the method definition.
| static <T> Result<T, RefreshErrorCode> success(T value) { | |
| return new Result<>(value, null, RefreshErrorCode.NONE, null); | |
| } | |
| static <T> Result<T> success(T value) { | |
| return new Result<>(value, null); | |
| static <T, E extends ErrorCode> Result<T, E> success(T value, E errorCode) { | |
| return new Result<>(value, null, errorCode, null); | |
| } | |
| static <T, E extends ErrorCode> Result<T, E> success(T value, E errorCode) { | |
| return new Result<>(value, null, errorCode, null); | |
| } |
Alternatively, we might replace the success methods with something like refreshSuccess and evaluationSuccess. However, this is my less preferred option.
| /** The refresh operation failed because an invalid HTTP response was received (200 OK with an invalid content). */ | ||
| INVALID_HTTP_RESPONSE_CONTENT(1105), | ||
|
|
||
| /** Initialization of the SDK timed out. **/ |
There was a problem hiding this comment.
| /** Initialization of the SDK timed out. **/ | |
| /** Client initialization could not complete within `maxInitWaitTimeSeconds`. **/ |
| private static <T> void validateReturnType(Class<T> classOfT) { | ||
| if (!(classOfT == String.class || classOfT == Integer.class || classOfT == int.class || classOfT == Double.class || classOfT == double.class || classOfT == Boolean.class || classOfT == boolean.class)) { | ||
| throw new IllegalArgumentException("Only String, Integer, Double or Boolean types are supported."); | ||
| throw new EvaluationException("Only String, Integer, Double or Boolean types are supported."); |
There was a problem hiding this comment.
This should remain an exception thrown early instead of being swallowed to be consistent with other SDKs. E.g.: https://github.com/configcat/.net-sdk/blob/v10.0.0/src/ConfigCatClient/Extensions/TypeExtensions.cs#L11
| throw new EvaluationException("Only String, Integer, Double or Boolean types are supported."); | |
| throw new IllegalArgumentException("Only String, Integer, Double or Boolean types are supported."); |
| try { | ||
| validateReturnType(classOfT); | ||
|
|
||
| return this.getSettingsAsync() | ||
| return this.getSettingsAsync() | ||
| .thenApply(settingsResult -> { | ||
| Result<Setting> checkSettingResult = checkSettingAvailable(settingsResult, key, defaultValue); | ||
| Result<Setting, EvaluationErrorCode> checkSettingResult = checkSettingAvailable(settingsResult, key, defaultValue); | ||
| if (checkSettingResult.error() != null) { | ||
| EvaluationDetails<Object> evaluationDetails = EvaluationDetails.fromError(key, defaultValue, checkSettingResult.error(), user); | ||
| EvaluationDetails<Object> evaluationDetails = EvaluationDetails.fromError(key, defaultValue, checkSettingResult.errorCode(), checkSettingResult.error(), null, user); | ||
| this.hooks.invokeOnFlagEvaluated(evaluationDetails); | ||
| return evaluationDetails.asTypeSpecific(); | ||
| } | ||
| return this.evaluate(classOfT, checkSettingResult.value(), | ||
| key, user != null ? user : this.defaultUser, settingsResult.fetchTime(), settingsResult.settings()); | ||
| }); | ||
| } catch (InvalidConfigModelException e) { | ||
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | ||
| return CompletableFuture.completedFuture(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.INVALID_CONFIG_MODEL, e.getMessage(), null, user)); | ||
| } catch (EvaluationException e) { | ||
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | ||
| return CompletableFuture.completedFuture(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.SETTING_VALUE_TYPE_MISMATCH, e.getMessage(),null, user)); | ||
| } catch (Exception e) { | ||
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | ||
| return CompletableFuture.completedFuture(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.UNEXPECTED_ERROR, e.getMessage(), e, user)); | ||
| } |
There was a problem hiding this comment.
There are multiple issues with this change:
- The exception thrown by
validateReturnType(classOfT);shouldn't be swallowed. (It should remain an early error.) - Wrapping the code that composes the completable future object won't catch exceptions thrown in the continuation callback scheduled by
thenApply. - Separately handling the various types of exceptions is redundant. This can be greatly simplified using a helper method. E.g.: https://github.com/configcat/.net-sdk/blob/v10.0.0/src/ConfigCatClient/Evaluation/EvaluationHelper.cs#L182-L190
invokeOnFlagEvaluatedshould also be called in case of error.
The correct error handling would be something like this:
| try { | |
| validateReturnType(classOfT); | |
| return this.getSettingsAsync() | |
| return this.getSettingsAsync() | |
| .thenApply(settingsResult -> { | |
| Result<Setting> checkSettingResult = checkSettingAvailable(settingsResult, key, defaultValue); | |
| Result<Setting, EvaluationErrorCode> checkSettingResult = checkSettingAvailable(settingsResult, key, defaultValue); | |
| if (checkSettingResult.error() != null) { | |
| EvaluationDetails<Object> evaluationDetails = EvaluationDetails.fromError(key, defaultValue, checkSettingResult.error(), user); | |
| EvaluationDetails<Object> evaluationDetails = EvaluationDetails.fromError(key, defaultValue, checkSettingResult.errorCode(), checkSettingResult.error(), null, user); | |
| this.hooks.invokeOnFlagEvaluated(evaluationDetails); | |
| return evaluationDetails.asTypeSpecific(); | |
| } | |
| return this.evaluate(classOfT, checkSettingResult.value(), | |
| key, user != null ? user : this.defaultUser, settingsResult.fetchTime(), settingsResult.settings()); | |
| }); | |
| } catch (InvalidConfigModelException e) { | |
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | |
| return CompletableFuture.completedFuture(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.INVALID_CONFIG_MODEL, e.getMessage(), null, user)); | |
| } catch (EvaluationException e) { | |
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | |
| return CompletableFuture.completedFuture(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.SETTING_VALUE_TYPE_MISMATCH, e.getMessage(),null, user)); | |
| } catch (Exception e) { | |
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | |
| return CompletableFuture.completedFuture(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.UNEXPECTED_ERROR, e.getMessage(), e, user)); | |
| } | |
| validateReturnType(classOfT); | |
| return this.getSettingsAsync() | |
| .thenApply(settingsResult -> { | |
| Result<Setting, EvaluationErrorCode> checkSettingResult = checkSettingAvailable(settingsResult, key, defaultValue); | |
| if (checkSettingResult.error() != null) { | |
| EvaluationDetails<Object> evaluationDetails = EvaluationDetails.fromError(key, defaultValue, checkSettingResult.errorCode(), checkSettingResult.error(), null, user); | |
| this.hooks.invokeOnFlagEvaluated(evaluationDetails); | |
| return evaluationDetails.asTypeSpecific(); | |
| } | |
| try { | |
| return this.evaluate(classOfT, checkSettingResult.value(), | |
| key, user != null ? user : this.defaultUser, settingsResult.fetchTime(), settingsResult.settings()); | |
| } catch (Exception e) { | |
| this.logger.error(1002, ConfigCatLogMessages.getSettingEvaluationErrorWithDefaultValue("getValueDetails", key, "defaultValue", defaultValue), e); | |
| EvaluationDetails<Object> evaluationDetails = EvaluationDetails.fromError(key, defaultValue, getEvaluationErrorCode(e), e.getMessage(), e, user); | |
| this.hooks.invokeOnFlagEvaluated(evaluationDetails); | |
| return evaluationDetails.asTypeSpecific(); | |
| } | |
| }); |
| try { | ||
| return forceRefreshAsync().get(); | ||
| } catch (InterruptedException e) { | ||
| logger.error(0, "Thread interrupted.", e); | ||
| Thread.currentThread().interrupt(); | ||
| } catch (Exception e) { | ||
| this.logger.error(1003, ConfigCatLogMessages.getForceRefreshError("forceRefresh"), e); | ||
| } | ||
| return new RefreshResult(false, "An error occurred during the refresh."); | ||
| return new RefreshResult(false, "An error occurred during the refresh.", RefreshErrorCode.UNEXPECTED_ERROR, null); |
There was a problem hiding this comment.
Let's include the exception in the result object following the pattern used in e.g. getValueDetails.
| this.hooks.invokeOnFlagEvaluated(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.UNEXPECTED_ERROR, error + " " + e.getMessage(), e, userObject)); | ||
| this.logger.error(2001, error, e); |
There was a problem hiding this comment.
This method should also handle the specific error cases (EvaluationException, InvalidConfigModelException).
Also, we should log the error first, as invokeOnFlagEvaluated calls user-provided code that might throw.
| this.hooks.invokeOnFlagEvaluated(EvaluationDetails.fromError(key, defaultValue, EvaluationErrorCode.UNEXPECTED_ERROR, error + " " + e.getMessage(), e, userObject)); | |
| this.logger.error(2001, error, e); | |
| this.logger.error(2001, error, e); | |
| this.hooks.invokeOnFlagEvaluated(EvaluationDetails.fromError(key, defaultValue, getEvaluationErrorCode(e), error + " " + e.getMessage(), e, userObject)); |
| return double.class; | ||
| else | ||
| throw new IllegalArgumentException("Only String, Integer, Double or Boolean types are supported"); | ||
| throw new EvaluationException("Only String, Integer, Double or Boolean types are supported"); |
There was a problem hiding this comment.
| throw new EvaluationException("Only String, Integer, Double or Boolean types are supported"); | |
| throw new IllegalArgumentException("Only String, Integer, Double or Boolean types are supported."); |
| FormattableLogMessage message = ConfigCatLogMessages.getFetchReceived200WithInvalidBodyError(cfRayId); | ||
| this.logger.error(1105, message, e); | ||
| return Result.error(message, null); | ||
| return Result.error(message, null, EvaluationErrorCode.INVALID_CONFIG_MODEL, null); |
There was a problem hiding this comment.
Let's keep it consistent with other SDKs. E.g.: https://github.com/configcat/.net-sdk/blob/v10.0.0/src/ConfigCatClient/ConfigService/DefaultConfigFetcher.cs#L73
| return Result.error(message, null, EvaluationErrorCode.INVALID_CONFIG_MODEL, null); | |
| return Result.error(message, null, EvaluationErrorCode.INVALID_HTTP_RESPONSE_CONTENT, null); |


Describe the purpose of your pull request
EvaluationErrorCodeandRefreshErrorCodeand exposed them through theEvaluationDetailsandRefreshResult.Related issues (only if applicable)
How to test? (only if applicable)
Security (only if applicable)
Requirement checklist (only if applicable)